Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

python/iot3: make OTLP optional in simple API #259

Merged

Conversation

ymorin-orange
Copy link
Member

@ymorin-orange ymorin-orange commented Jan 14, 2025

Changes:

  • Feature: allow using the simple API without OTLP

Closes: #236


How to test:

  1. Pre-requisites:
    • an OpenTelemetry collector on localhost:
      $ docker container run \
          --rm \
          -p 16686:16686 \
          -p 4318:4318 \
          jaegertracing/all-in-one:1.58
    • a python 3.11 environment; if you do not have one already, use a container:
      $ docker container run \
          --detach \
          --name iot3 \
          --rm \
          -ti \
          --network host \
          -e http_proxy \
          -e https_proxy \
          -e no_proxy \
          --user $(id -u):$(id -u) \
          --mount type=bind,source=$(pwd),destination=$(pwd) \
          --workdir $(pwd) \
          python:3.11.9-slim-bookworm \
          /bin/bash -il
      $ docker container exec -u 0:0 iot3 apt update
      $ docker container exec -u 0:0 iot3 apt install -y git
      $ docker container attach iot3
      Note: in the following tests, the $ prompt means running in a shell that has access to python3.11, while the 🐍 $ prompt means running in the activated venv.
  2. Install the IoT3 SDK package:
    $ python3.11 -m venv /tmp/iot3
    $ . /tmp/iot3/bin/activate
    🐍 $ pip install python/iot3
  3. Run the IoT3 test-suite for the simple API:
    🐍 $ python/iot3/tests/test-iot3-core
    

Expected results:

  1. The IoT3 SDK package is isntalled
  2. The test-suite succeeds:
    • the test reports ([...] is a hex string):
      IoT3 Core SDK with MQTT and OTLP
      test/[...]/iot3/passed: b'passed'
      IoT3 Core SDK with MQTT and no OTLP
      test/[...]/iot3/no-otlp/passed: b'passed'
      
    • The Jaeger UI (on http://localhost:16686/) contains exactly three traces which MQTT topic (in Tags -> iot3.core.mqtt.topic) does not contain the no-otlp component:
      • one trace reports a failure due to not being connected;
      • one trace reports a publish;
      • one trace reports a receive with a reference to the publish, above.

@ymorin-orange ymorin-orange force-pushed the nyma/gh-236.optional-otlp branch from 505e063 to 52aa2d9 Compare January 14, 2025 12:35
@ymorin-orange
Copy link
Member Author

The rust workflow fails for reasons unrelated to this PR, as no rust code was touched here.

@ymorin-orange ymorin-orange marked this pull request as ready for review January 14, 2025 14:54
@Hugues360
Copy link
Collaborator

A very little typo in commit description "python/iot3/core: allow creating an MQTT client with explicitly no OTLP" : 'rahter' instead of 'rather'

@Hugues360
Copy link
Collaborator

In the "How to test" section of PR description, the OpenTelemetry collector has to be launched in a shell that don't have to have necessarly access to python 3.11, which is not what is said in the "Note" above. In fact if we have used the commands above to have acces to a python 3.11 environment, the command of Opentelemetry launch will fail as there is no docker on the python:3.11.9-slim-bookworm used. I think that the command about OpenTelemetry launch should be put before the one of python 3.11 environment to avoid misunderstanding and be coherent with the 'Note'.

@Hugues360
Copy link
Collaborator

Hugues360 commented Jan 16, 2025

In the "Expected results" section, I don't have the logs "test/[...]/iot3/passed: b'passed'" in the shell where I launch the test, neither "test/[...]/iot3/no-otlp/passed: b'passed'

@ymorin-orange ymorin-orange force-pushed the nyma/gh-236.optional-otlp branch from 52aa2d9 to 2ff5212 Compare January 16, 2025 14:48
@ymorin-orange
Copy link
Member Author

In the "Expected results" section, I don't have the logs "test/[...]/iot3/passed: b'passed'" in the shell where I launch the test, neither "test/[...]/iot3/no-otlp/passed: b'passed'

If you are behind the VPN on the internal network, you will not be able to connect to the MQTT broker on test.mosquitto.org. Do your tests either directly on an unfiltered internet, or add proper routes.

@Hugues360
Copy link
Collaborator

In the "Expected results" section, I don't have the logs "test/[...]/iot3/passed: b'passed'" in the shell where I launch the test, neither "test/[...]/iot3/no-otlp/passed: b'passed'

If you are behind the VPN on the internal network, you will not be able to connect to the MQTT broker on test.mosquitto.org. Do your tests either directly on an unfiltered internet, or add proper routes.

Ah ok, thank you. it works now even if during my first test, I had 2 failure traces in OTLP and no log for the "with otlp" test, certainly the time.sleep of 1 second can be to short... an attempt with setting timer after dropped test to 3s works and further tests with the timer at 1s worked too.

... and fix my own identity while at it.

Fixes: Orange-OpenSource#258

Signed-off-by: Yann E. MORIN <yann.morin@orange.com>
Signed-off-by: Yann E. MORIN <yann.morin@orange.com>
Signed-off-by: Yann E. MORIN <yann.morin@orange.com>
Currently, we can create an MQTT client either by passing an explicit
OTLP span manager, whether real or fake, or by not passing anything, in
which case no OTLP traces would be sent.

However, for some callers, it might be simpler to explicitly pass 'None'
to not send OTLP traces, rather than pass a fake span manager.

Change the MQTT signature to accept this new use-case, in a backward
compatible way.

Signed-off-by: Yann E. MORIN <yann.morin@orange.com>
Currently, using OTLP is mandatory in the simple API. However, it is
perfectly legit for a user to only be interested in sending MQTT
messages.

Similarly, it is possible that a bootstrap reply does not contain any
OTLP setup when the IoT3 instance does not offer an OTLP collector

Signed-off-by: Yann E. MORIN <yann.morin@orange.com>
We currently use a loop to iterate over all the possible compression and
authentication schemes, and compare their string representation against
the valude from the configuration, which is a string.

However, python's enum.strEnum() constructor does accept a string as a
parameter, where that string is the str() representation of one of the
enum values, which incidentally also plays the role of validating the
value from the configuration.

Use that, rather than our canned, ugly for-loop.

Signed-off-by: Yann E. MORIN <yann.morin@orange.com>
Signed-off-by: Yann E. MORIN <yann.morin@orange.com>
@ymorin-orange ymorin-orange force-pushed the nyma/gh-236.optional-otlp branch from 2ff5212 to 91b9356 Compare January 17, 2025 08:29
@ymorin-orange ymorin-orange merged commit 06a0574 into Orange-OpenSource:master Jan 17, 2025
58 checks passed
@ymorin-orange ymorin-orange deleted the nyma/gh-236.optional-otlp branch January 17, 2025 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make telemetry optionnal
2 participants